Report unhappy blocks in happy path test #430
Conversation
nomos-core/src/block/debug_happy.rs
Outdated
| impl<Tx: Clone + Eq + Hash, Blob: Clone + Eq + Hash> Drop for super::Block<Tx, Blob> { | ||
| fn drop(&mut self) { | ||
| let header = self.header(); | ||
| // We need to persist the block to disk before dropping it | ||
| // if there is a aggregated qc when testing happy path | ||
| if let Qc::Aggregated(qcs) = &header.parent_qc { | ||
| #[derive(serde::Serialize)] | ||
| struct Info<'a> { | ||
| id: String, | ||
| view: View, | ||
| qcs: &'a AggregateQc, | ||
| } | ||
|
|
||
| let mut file = debug_file().lock(); | ||
| let info = Info { | ||
| id: format!("{}", header.id), | ||
| view: header.view, | ||
| qcs, | ||
| }; | ||
| // Use pretty print to make it easier to read, because we need the this for debugging. | ||
| serde_json::to_writer_pretty(&mut *file, &info).unwrap(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this should be a simple method or function and not do it in drop. It is better to see the call explicetely it in the test.
Yeah, we have |
tests/src/tests/happy.rs
Outdated
| pub fn persist(name: &'static str, blocks: impl Iterator<Item = &Block>) { | ||
| // We need to persist the block to disk before dropping it | ||
| // if there is a aggregated qc when testing happy path | ||
| if let Qc::Aggregated(qcs) = &block.parent_qc { | ||
| #[derive(serde::Serialize)] | ||
| struct Info<'a> { | ||
| id: String, | ||
| view: View, | ||
| qcs: &'a AggregateQc, | ||
| } | ||
|
|
||
| let mut file = OpenOptions::new() | ||
| .create(true) | ||
| .truncate(true) | ||
| .read(true) | ||
| .write(true) | ||
| .open(format!("{name}.json")) | ||
| .unwrap(); | ||
| let infos = blocks | ||
| .map(|b| Info { | ||
| id: format!("{}", header.id), | ||
| view: header.view, | ||
| qcs, | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| // Use pretty print to make it easier to read, because we need the this for debugging. | ||
| serde_json::to_writer_pretty(&mut *file, &infos).unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we should persist them, here are the steps I think need to be done:
- We can add a query for requesting the blocks to the node.
- Before killing the nodes, retrieve blocks.
- Check blocks with this function (or similar).
- Report
NodeIdBlockIdViewfor every block that is not starndardQc
There was a problem hiding this comment.
We already have an api to get blocks from the node, I think we can reuse that. In fact, it's the same that we use to check the view, it also has an additional field with blocks info
There was a problem hiding this comment.
We already have an api to get blocks from the node, I think we can reuse that. In fact, it's the same that we use to check the view, it also has an additional field with blocks info
Even better!
What do you mean? |
Ah, I think there may be some "intermediate" blocks, and we also want to check it, so I just implemented a drop method for it . |
All intermediate blocks are in safe blocks apart from outright malicious / invalid ones |
danielSanchezQ
changed the title
* Use tree overlay in nomos node * Use tree overlay in tests module * Handle unexpected consensus vote stream end in tally * Spawn the next leader first (#425) * Report unhappy blocks in happy path test (#430) * report unhappy blocks in the happy path test * Make threshold configurable for TreeOverlay (#426) * Modified test, so that all nodes don’t all connect to the first node only (#442) * merge fix --------- Co-authored-by: Youngjoon Lee <[email protected]> Co-authored-by: Al Liu <[email protected]>
As mentioned in the meeting, we want to make sure in happy path tests, if a block contains AggratedQc, then we need to persist the block information.
I implemented a
dropmethod forBlockand feature gate it, this way we do not need to modify any of our current codebase, and we can easily remove the code and feature in the future when we fully solve the problem.